Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: grc20 dynamic call with registered interface #1275

Closed
wants to merge 8 commits into from
Closed

feat: grc20 dynamic call with registered interface #1275

wants to merge 8 commits into from
+551 −0

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Oct 21, 2023

allow certain contract to call grc20 functions(Transfer,TransferFrom,BalanceOf) without import by using registered interface

// co-worker @mconcat

relate pr

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@r3v4s r3v4s requested a review from a team as a code owner October 21, 2023 15:22
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Oct 21, 2023
@codecov
Copy link

codecov bot commented Oct 21, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 56.66%. Comparing base (a2b5f29) to head (36ef3bb).
Report is 596 commits behind head on master.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/realm.go 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1275      +/-   ##
==========================================
+ Coverage   56.17%   56.66%   +0.49%     
==========================================
  Files         439      425      -14     
  Lines       66242    64786    -1456     
==========================================
- Hits        37209    36709     -500     
+ Misses      26143    25225     -918     
+ Partials     2890     2852      -38
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@n0izn0iz
Copy link
Contributor

Also related to #1072 I believe

@dongwon8247 dongwon8247 mentioned this pull request Oct 21, 2023
Open
18 tasks
@michelleellen michelleellen mentioned this pull request Nov 1, 2023
Open
@michelleellen michelleellen mentioned this pull request Nov 29, 2023
Open
@dongwon8247 dongwon8247 mentioned this pull request Dec 6, 2023
Open
@harry-hov
Copy link
Contributor

@r3v4s you might need to run update gno.mod files. CI will start complaining once you rebase it with master

You have 4 ways:

Option1:

Check imports and update each `gno.mod` manually

Option2:

Run `gno mod tidy` per package.

Option3:

$ cd examples/gno.land/r/x/grc20_dynamic_call
$ find . -name "gno.mod" -execdir gno mod tidy \;

Option4:

$ cd examples
$ make tidy

I would personally prefer Option4 :)

@r3v4s r3v4s force-pushed the feat/grc20-dynamic-call-with-registered-interface branch from 537bf53 to db05596 Compare December 7, 2023 05:08
@r3v4s r3v4s marked this pull request as draft December 7, 2023 05:14
@r3v4s
Copy link
Contributor Author

r3v4s commented Dec 7, 2023

converting to draft to fix testcase

zivkovicmilos
zivkovicmilos approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this detailed PR 🙏

Please check out @harry-hov's comments about the unused import, otherwise it looks good 💯

examples/gno.land/r/x/grc20_dynamic_call/foo/foo.gno Outdated Show resolved Hide resolved
examples/gno.land/r/x/grc20_dynamic_call/registry/registry.gno Outdated Show resolved Hide resolved
moul
moul reviewed Dec 7, 2023
View reviewed changes
examples/gno.land/r/x/grc20_dynamic_call/baz/baz.gno Outdated
@@ -0,0 +1,130 @@
package baz

import (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using either a bar or baz instead of exposing public methods. Create a safe object with the following code:

var baz = grc20.NewAdminToken(...)
var Baz = baz.GRC20()
// call Baz.Approve, etc directly

This recommendation reduces the codebase and eliminates redundancy. It demonstrates that the registry can function with standard public-facing grc20 implementations or grc20 objects. Ideally, keep a single foo file with public methods, while keeping the other files around 30 lines long. This approach will make the example more powerful and concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling Baz.Transfer() does panic VM call panic: cannot modify external-realm or non-realm object because of updating its token balance.

as @tbruyelle mentioned, #1257 fix need to be settle down for this pr to use grc20 object instead of public methods

@thehowl thehowl mentioned this pull request Dec 13, 2023
Open
@tbruyelle
Copy link
Contributor

tbruyelle commented Jan 12, 2024 •
edited
Loading

@r3v4s Why do you need this wrapper ? Why not calling directly register in the related contract (foo/bar/baz) ?

Is this to avoid this error : VM call panic: cannot modify external-realm or non-realm object ?

Because the bug behind this error is fixed here #1257.

@go7066 go7066 mentioned this pull request Feb 2, 2024
Open
@r3v4s r3v4s force-pushed the feat/grc20-dynamic-call-with-registered-interface branch from 900da31 to 36ef3bb Compare February 13, 2024 12:19
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Feb 13, 2024
@r3v4s r3v4s mentioned this pull request Mar 5, 2024
Open
@dongwon8247 dongwon8247 mentioned this pull request Mar 25, 2024
Open
@n0izn0iz n0izn0iz mentioned this pull request Jul 5, 2024
Closed
7 tasks
@moul moul mentioned this pull request Jul 5, 2024
Draft
@thehowl
Copy link
Member

thehowl commented Nov 2, 2024

Is this PR still relevant?

@r3v4s
Copy link
Contributor Author

r3v4s commented Nov 3, 2024

Is this PR still relevant?

#2516 implements more simple way, therefore closing this issue.

@r3v4s r3v4s closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harry-hov harry-hov harry-hov left review comments

@moul moul moul left review comments

@zivkovicmilos zivkovicmilos zivkovicmilos approved these changes

@gfanton gfanton Awaiting requested review from gfanton

Assignees

@r3v4s r3v4s

Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
💪 Bounties & Worx
Status: Done
🚀 The Launch [DEPRECATED]
Status: 🌟 Wanted for Launch
🤝🏻 Partner: Onbloc
Status: Done
🧐 Review Meeting
Status: No status
🧙‍♂️gno.land core team
Status: Done
Milestone
🏗4️⃣ test4.gno.land [POST LAUNCH]
Development

Successfully merging this pull request may close these issues.
8 participants
@r3v4s @n0izn0iz @harry-hov @tbruyelle @thehowl @moul @zivkovicmilos @Kouteki